perf(src/index): regression caused by parsing source maps individually #287
Conversation
|
Jason Johnston seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
alexander-akait
left a comment
alexander-akait
left a comment
There was a problem hiding this comment.
Bad solution avg on memory usage increase on ~25%, let use map instead weakmap
|
If you feel this was a "bad" solution, then you really won't like using a I'm not sure exactly how you're measuring "avg on memory usage", or what baseline you're comparing against. |
|
@lojjic local tests, in near future i will do pr with benchmark npm command. We need source map only when we have warnings/errors. Right now your solution always try to parse source maps even i don't have warning and error. I.E. increase memory/cpu usage when i don't need this. |
|
Just replace |
That is not true, check the diff. Line 201 after the change. |
| warning, | ||
| file, | ||
| inputSourceMap, | ||
| sourceMap, |
There was a problem hiding this comment.
Also sourceMap can be undefined here, because warning !== error (and parsing in catch block doesn't execute)
There was a problem hiding this comment.
/cc @lojjic yep, my mistake, but if no error(s) - no source map
There was a problem hiding this comment.
again mistake, sorry, very tired
There was a problem hiding this comment.
I'm not following you. sourceMap can be null, if there is no valid inputSourceMap, but that's expected AFAICT.
alexander-akait
left a comment
alexander-akait
left a comment
There was a problem hiding this comment.
Hm, looks very good, let's add test:
- source map is good and we have warning
- source map is broken and we have warning
- source map is good and we have error
- source map is broken and we have error
To ensure we have expected behaviour
|
Also i investigate why benchmark increase memory usage on |
michael-ciniawsky
changed the title
|
|
||
| tasks.push(task); | ||
| } catch (error) { | ||
| let sourceMap = null; |
| const { error, map, code, warnings, extractedComments } = data; | ||
|
|
||
| // Create source map if needed | ||
| let sourceMap = null; |
michael-ciniawsky
changed the title
|
/cc @lojjic friendly ping |
|
Thanks for the bump. I understand the desire to fill in the test coverage for those scenarios you mentioned, if they were previously missing. I've glanced at the tests and I'd definitely have to come up to speed with how the whole plugin works in context as well as how your tests are structured. Given my current time constraints I wouldn't expect that to happen in the next couple weeks. |
|
@lojjic thanks for answer, i will try to find time on writing tests |
|
Close in favor #305 Thanks! |
4 participants
See issue #286
This approach reverts to not using a separate cache object, but being careful about lazily instantiating the
SourceMapConsumeronly when we know it is needed.In my project's build, this took the number of instantiated
SourceMapConsumerobjects from 1418 down to 21.